Native Animated - Support events using RCT{Direct|Bubbling}EventBlock on iOS by janicduplessis · Pull Request #15611 · react/react-native

janicduplessis · 2017-08-23T00:35:07Z

When calling a prop of type RCTDirectEventBlock or RCTBubblingEventBlock it uses a completely different code path than events using [RCTEventDispatcher sendEvent:] and those were not dispatched to the RCTEventDispatcherListeners. We also do some event name normalization which caused issues between the JS and native event names. To fix that I simply remove the parts we normalize from the event key.

Changelog:

[iOS] [Fixed] - Support events using RCT{Direct|Bubbling}EventBlock

Test plan:

Added a Slider (it used RCTBubblingEventBlock for it's onValueChange event) that can control a native animated value in RNTester to reproduce the bug and made sure this diff fixes it.

pull-bot · 2017-08-23T00:51:37Z

@facebook-github-bot label Core Team

Attention: @janicduplessis, @shergin

Generated by 🚫 dangerJS

shergin

Looks like two great PR mixed up here. 😄

shergin · 2017-08-23T05:23:54Z

+- (instancetype)initWithName:(NSString *)name viewTag:(NSNumber *)viewTag arguments:(NSArray *)arguments
+{
+  self = [super init];
+  if (self) {


One line, please.

shergin · 2017-08-23T05:27:54Z

  );
 }

+/**


Separate file, please.

janicduplessis · 2017-08-23T06:14:57Z

@shergin Updated

nerdmed · 2017-08-23T07:41:32Z

@janicduplessis this is awesome! Looking forward for this!

shergin · 2017-08-23T21:05:25Z

@janicduplessis What does RCTInputEvent and Animated have in common?

janicduplessis · 2017-08-23T21:33:57Z

@shergin Ok I understand why you are confused :)

Not sure why it is called INPUT event because afaik it has nothing to do with inputs, I just kept the current naming.

So when using RCTDirectEventBlock to create a view manager prop it ends up here https://github.com/facebook/react-native/blob/master/React/Views/RCTComponentData.m#L242 then createEventSetter will create a block that calls into [sendInputEventWithName:] (https://github.com/facebook/react-native/blob/master/React/Views/RCTComponentData.m#L121) to dispatch the event to JS. We want to call the listeners there too like we do in [sendEvent:] so native animated can do its thing.

janicduplessis · 2017-08-23T21:41:26Z

And the other part of the PR is to deal with the event name normalization that is done in sendInputEventWithName (https://github.com/facebook/react-native/blob/master/React/Base/RCTEventDispatcher.m#L91)

shergin · 2017-08-24T00:12:01Z

OMG, now I know how RCTDirectEvent works. Almost. Thanks!
I will try to figure it out. We have to split this anyways though.

janicduplessis · 2017-08-24T02:57:40Z

@shergin 👍 Let me know if you need more context on anything else.

shergin · 2017-08-25T02:41:13Z

@janicduplessis Could you please help me figure out why this was named sendInputEvent? Is it just because it was used for TextInput only?
Should we generalize this approach? And rename it?

janicduplessis · 2017-08-25T03:17:03Z

@shergin From doing a quick search looks like it is only used in createEventSetter and sendTextEventWithType. It has been deprecated for a while (https://github.com/facebook/react-native/blob/e1577df1fd70049ce7f288f91f6e2b18d512ff4d/React/Base/RCTEventDispatcher.h#L87) so changing / removing it should be fine.

There is also sendEvent which takes a RCTEvent and is similar to sendInputEvent but more complex, It uses an event queue and does optional coalescing. Ideally we'd always use sendEvent but I don't know if the scheduling would end up being exactly the same.

shergin · 2017-08-25T05:09:02Z

@janicduplessis I just want to know that we know what we are doing. And do the right things. 😄
I would love to adopt sendEvent... everywhere!

janicduplessis · 2017-08-25T07:29:20Z

@shergin Ok, I had a better look at that code and it should be possible to switch to using only sendEvent. There's a bit more overhead but it should not make a noticeable difference performance wise. Some of our more frequent events like onScroll already use sendEvent anyway.

I can put up a PR tomorrow to remove sendInputEvent and move everything to sendEvent.

shergin · 2017-08-25T16:06:27Z

@janicduplessis Great! Do you mean remove sendInputEvent from Animated of remove completely?

janicduplessis · 2017-08-25T17:33:39Z

I think we can get rid of it completely and always use sendEvent

janicduplessis · 2017-08-25T22:37:35Z

@shergin Removed sendInputEvent and created a new RCTEvent class that can be used with sendEvent and that replicates the behaviour of sendInputEvent. Named it RCTComponentEvent since sendInputEvent was used to dispatch events to components (including but not limited to text input events). That name should make it a lot clearer what it is used for.

Also simplified dealing with non-coalescable events by automatically generating a coalescingKey is canCoalesce returns NO. Also made a few coalescing related methods/props optional so they can be omitted if canCoalesce is false. Otherwise every RCTEvent class would have to implement some unique key generator even if the event is never coalescable.

janicduplessis · 2017-08-25T22:39:00Z

Also this should probably be split in 2 PRs, just wanted to keep it here for now but once we're happy with a solution I'll make a new PR for the removal of sendInputEvent.

nerdmed · 2017-09-29T07:49:50Z

@janicduplessis @shergin any news here? What is needed to proceed with this pr?

shergin · 2017-09-29T21:10:43Z

    event.viewTag.intValue |
    (((uint64_t)event.eventName.hash & 0xFFFF) << 32) |
-    (((uint64_t)event.coalescingKey) << 48)
+    (((uint64_t)([event canCoalesce] ? event.coalescingKey : RCTNonCoalescableEventKey++)) << 48)


We access optional canCoalesce and coalescingKey here.
So, we have two options here:

Do responseToSelector check here.

Make that props required for all implementations.

I strongly prefer 2 because we should maintain this function as perfromant as possible.

Right?

Oh, no, I see, I am wrong.

shergin · 2017-09-30T21:39:25Z

@janicduplessis Okay, we will be back to this PR after landing of #15894. Right?

janicduplessis · 2017-09-30T21:45:47Z

@shergin Yep, then I will rebase this PR on top of the event improvements

facebook-github-bot · 2017-10-16T17:06:22Z

@janicduplessis I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

nerdmed · 2018-02-02T16:29:51Z

@janicduplessis thanks a lot for your previous effort on this PR. It would enable really nice native animations with RN. This become stale for more a couple of month now. Any change to get this going?

pull-bot · 2019-04-22T00:44:33Z

	Messages
📖	📋 Missing Summary - Can you add a Summary? To do so, add a "## Summary" section to your PR description. This is a good place to explain the motivation for making this change.

Generated by 🚫 dangerJS against 5967a1d75c7c6ee87ab93c8464d1550c3cad29b3

janicduplessis · 2019-04-22T00:47:13Z

@shergin Rebased and tested it still works :)

cpojer · 2019-06-04T13:32:00Z

What's the status of this PR?

… on iOS

pull-bot · 2019-06-19T03:20:44Z

	Messages
📖	📋 Missing Summary - Can you add a Summary? To do so, add a "## Summary" section to your PR description. This is a good place to explain the motivation for making this change.

Generated by 🚫 dangerJS against 4fe14cf

janicduplessis · 2019-06-19T03:27:18Z

@cpojer @shergin Rebased this and made sure the fix is still needed and works.

cpojer

Let's ship it.

facebook-github-bot

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

react-native-bot · 2019-06-19T08:50:07Z

This pull request was successfully merged by @janicduplessis in 083f835.

^{When will my fix make it into a release? | Upcoming Releases}

cpojer · 2019-06-19T09:56:20Z

Had to revert this because of:

react-native/Libraries/NativeAnimation/RCTNativeAnimatedNodesManager.m:35:11: error: no previous prototype for function 'RCTNormalizeAnimatedEventName' [-Werror,-Wmissing-prototypes]
NSString *RCTNormalizeAnimatedEventName(NSString *eventName)
          ^

Could you send it again with this fixed?

janicduplessis · 2019-06-19T18:11:58Z

@cpojer Fixed #25317

janicduplessis requested review from javache and shergin as code owners August 23, 2017 00:35

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Aug 23, 2017

shergin suggested changes Aug 23, 2017

View reviewed changes

nerdmed mentioned this pull request Aug 23, 2017

useNativeDriver for onProgress event TheWidlarzGroup/react-native-video#711

Closed

hramos added the Core Team label Aug 24, 2017

shergin reviewed Sep 29, 2017

View reviewed changes

hramos removed the GH Review: review-needed label Mar 8, 2018

janicduplessis force-pushed the native-events-fix branch from 0726310 to c2f72e3 Compare April 22, 2019 00:42

react-native-bot added Includes Changelog Bug and removed Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. labels Apr 22, 2019

shergin suggested changes Apr 29, 2019

View reviewed changes

Comment thread Libraries/NativeAnimation/RCTNativeAnimatedNodesManager.m Outdated

hramos removed Includes Changelog labels May 6, 2019

janicduplessis added 2 commits June 18, 2019 23:18

Native Animated - Support events using RCT{Direct|Bubbling}EventBlock…

5fd15b3

… on iOS

Fix typo

4fe14cf

janicduplessis force-pushed the native-events-fix branch from 5967a1d to 4fe14cf Compare June 19, 2019 03:18

cpojer approved these changes Jun 19, 2019

View reviewed changes

facebook-github-bot approved these changes Jun 19, 2019

View reviewed changes

facebook-github-bot closed this in 083f835 Jun 19, 2019

react-native-bot added the Merged This PR has been merged. label Jun 19, 2019

janicduplessis mentioned this pull request Jun 19, 2019

Native Animated - Support events using RCT{Direct|Bubbling}EventBlock on iOS #25317

Closed

janicduplessis deleted the native-events-fix branch June 19, 2019 18:13

simonbuerger mentioned this pull request Jul 1, 2019

Declarative onLayout software-mansion/react-native-reanimated#79

Closed

                 );
               }
+              /**

Uh oh!

Conversation

janicduplessis commented Aug 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog:

Test plan:

Uh oh!

pull-bot commented Aug 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shergin left a comment

Choose a reason for hiding this comment

Uh oh!

shergin Aug 23, 2017

Choose a reason for hiding this comment

Uh oh!

shergin Aug 23, 2017

Choose a reason for hiding this comment

Uh oh!

janicduplessis commented Aug 23, 2017

Uh oh!

nerdmed commented Aug 23, 2017

Uh oh!

shergin commented Aug 23, 2017

Uh oh!

janicduplessis commented Aug 23, 2017

Uh oh!

janicduplessis commented Aug 23, 2017

Uh oh!

shergin commented Aug 24, 2017

Uh oh!

janicduplessis commented Aug 24, 2017

Uh oh!

shergin commented Aug 25, 2017

Uh oh!

janicduplessis commented Aug 25, 2017

Uh oh!

shergin commented Aug 25, 2017

Uh oh!

janicduplessis commented Aug 25, 2017

Uh oh!

shergin commented Aug 25, 2017

Uh oh!

janicduplessis commented Aug 25, 2017

Uh oh!

janicduplessis commented Aug 25, 2017

Uh oh!

janicduplessis commented Aug 25, 2017

Uh oh!

nerdmed commented Sep 29, 2017

Uh oh!

shergin Sep 29, 2017

Choose a reason for hiding this comment

Uh oh!

shergin Sep 29, 2017

Choose a reason for hiding this comment

Uh oh!

shergin commented Sep 30, 2017

Uh oh!

janicduplessis commented Sep 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Oct 16, 2017

Uh oh!

nerdmed commented Feb 2, 2018

Uh oh!

pull-bot commented Apr 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janicduplessis commented Apr 22, 2019

Uh oh!

Uh oh!

cpojer commented Jun 4, 2019

Uh oh!

pull-bot commented Jun 19, 2019

Uh oh!

janicduplessis commented Jun 19, 2019

Uh oh!

cpojer left a comment

Choose a reason for hiding this comment

janicduplessis commented Aug 23, 2017 •

edited

Loading

pull-bot commented Aug 23, 2017 •

edited

Loading

janicduplessis commented Sep 30, 2017 •

edited

Loading

pull-bot commented Apr 22, 2019 •

edited

Loading